Skip to content

Conversation

@mergify
Copy link

@mergify mergify bot commented Jul 28, 2025

There were two problems in the codebase:

  1. timeline._update_index_cache was called per invalidated topic, which resulted in multiple reads of the whole bagfile (each topic seeks the reader to the bag start and forces it to read throughout the whole bag). There might be some caching on the way so that rqt_bag doesn't read 20x12 GB for a 12 GB file with 20 topics, but it did definitely read much more than 12 GB.
  2. Once I fixed the above to read all topics at once, another bug popped up: the bag entries were cached in memory so that they could be sorted before yielding them from BagTimeline.get_entries(). This was a smaller problem in the per-topic scenario, but a big problem in the all-topic one. But even in the per-topic scenario, rqt_bag needlessly cached gigabytes of entries (e.g. for an image topic) just to sort them.

This PR fixes both issues by utilizing a set of generators, one per bagfile, which produce messages simultaneousely and the earliest of all messages is taken.

I also got the progressbar working much more nicely (however, it only seems to work when loading the first bag).

Tests

The tests were performed on a 12 GB MCAP bag. Before each test, the bag file was evicted from FS cache.

I also wanted to test on a 70 GB bag file, but I didn't have enough hours and RAM to wait until it loads from a USB3 SSD. With this PR, it should be easy to open it (however, I can only verify that tomorrow).

Without this PR

real    1m7.152s
user    0m25.110s
sys     0m32.818s
Obrazovkove.vysilani.2025-05-08.13.49.15.mp4

With this PR

real    0m33.936s
user    0m16.651s
sys     0m7.837s
Obrazovkove.vysilani.2025-05-08.13.47.10.mp4

The problem mentioned in #166 that clicking on the timeline is super slow on large bags, still remains, however. But that's for a future PR.


This is an automatic backport of pull request #178 done by Mergify.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
(cherry picked from commit c7d3efd)
@ahcorde
Copy link
Contributor

ahcorde commented Jul 28, 2025

Pulls: #200
Gist: https://gist.githubusercontent.com/ahcorde/58b7d627eecf84f3f0c10ba6ad59d565/raw/24bcdeafe5bf88eb266acf574c837f39e2fedbcf/ros2.repos
BUILD args: --packages-above-and-dependencies rqt_bag
TEST args: --packages-above rqt_bag
ROS Distro: kilted
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16634

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit 735dce2 into kilted Jul 28, 2025
1 check passed
@ahcorde ahcorde deleted the mergify/bp/kilted/pr-178 branch July 28, 2025 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants